[CORE-149029] Fix preferred affinity restart loop caused by atomic list merge semantics#269
Open
dobrerazvan wants to merge 6 commits into
Open
[CORE-149029] Fix preferred affinity restart loop caused by atomic list merge semantics#269dobrerazvan wants to merge 6 commits into
dobrerazvan wants to merge 6 commits into
Conversation
PreferredDuringSchedulingIgnoredDuringExecution has no patchMergeKey in the Kubernetes API types, so the three-way strategic merge patch treats it as an atomic list. Any term injected by an admission controller (e.g. ScaleOps) is removed on every reconcile because the list in the running pod no longer matches the desired spec. Replace the previous sync-copy approach (which corrupted the last-applied annotation and silently suppressed intentional CR changes) with a CalculateOption that strips preferred affinities from both sides of the diff before the merge is computed. The diff becomes blind to those terms, so admission-controller additions are never fought. Also adds annotation backfill for broker pods that pre-date the banzaicloud.com/last-applied annotation: without it the three-way merge degenerates to a two-way diff, producing false resource diffs. Both behaviours are gated on the new spec.admissionWebhooksEnabled field. 12 unit/integration tests added covering deletePreferredAffinities and the full diff path via patch.DefaultPatchMaker.Calculate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…itionally ignorePreferredAffinities is a reconciler correctness fix, not a feature toggle. The atomic-list issue affects any external actor that touches preferredDuringSchedulingIgnoredDuringExecution, not just ScaleOps. The toleration merge already follows this precedent unconditionally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y current code Every pod Koperator creates goes through the creation path which calls SetLastAppliedAnnotation before r.Create. The annotation is therefore always present on pods managed by a current build; the backfill is dead code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… param - Extract affinityKeyPod/AntiPod/Node constants to satisfy goconst - Change baseKafkaPod() to take no arguments to satisfy unparam Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem: When ScaleOps (or any admission controller) injects preferredDuringSchedulingIgnoredDuringExecution terms, Koperator restarts brokers in a loop. Root cause: those lists have no patchMergeKey in the k8s API types → strategic merge patch treats them atomically → external additions are removed on every reconcile. Same class of bug as tolerations (which already has an explicit fix).
Secondary issue: pods without the banzaicloud.com/last-applied annotation fall back to two-way merge, which also reverts admission-controller resource-request changes.
Fix:
Required and resource-request management are unchanged. 12 new tests covering pure unit cases and full diff-level behaviour.
Type of Change
Checklist